-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
transaction history: add input addresses, metadata and slots filter #173
Conversation
4cac134
to
c24477a
Compare
webserver/server/app/models/transaction/sqlHistoryForAddresses.sql
Outdated
Show resolved
Hide resolved
webserver/server/app/controllers/TransactionHistoryController.ts
Outdated
Show resolved
Hide resolved
webserver/server/app/controllers/TransactionHistoryController.ts
Outdated
Show resolved
Hide resolved
webserver/server/app/controllers/TransactionHistoryController.ts
Outdated
Show resolved
Hide resolved
@@ -87,11 +96,36 @@ export class TransactionHistoryController extends Controller { | |||
}); | |||
} | |||
|
|||
let pageStartWithSlot = pageStart; | |||
|
|||
if (requestBody.slotLimits) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We really should have comments in the code for things like this otherwise it makes it takes a while to understand what is going on
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added some comments already, not sure if now it's clear enough.
webserver/server/app/models/pagination/slotBoundsPagination.sql
Outdated
Show resolved
Hide resolved
webserver/server/app/models/pagination/slotBoundsPagination.sql
Outdated
Show resolved
Hide resolved
address.free(); | ||
paymentCred?.free(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fyi you no longer need to free manually with CML v4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good to know, but carp is still on 3.0.1
(
carp/webserver/server/package.json
Line 28 in f44fd33
"@dcspark/cardano-multiplatform-lib-nodejs": "3.0.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, it will switch with this PR: #159
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's currently only for the indexer though, I don't see a change in the webserver version there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we'll want to upgrade it (maybe in a separate PR)
moved to the client
Co-authored-by: Sebastien Guillemot <[email protected]>
20ea85d
to
cde3ec4
Compare
FROM | ||
"Block" | ||
WHERE | ||
slot <= :low! AND tx_count > 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be slot >= :low!
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. This is used later as the :after_tx_id!
(when the pagination after parameter is not present). And that parameter is used as "Transaction".id > :after_tx_id!
.
Also, the lower bound of the slot range is supposed to be exclusive. Although I've been actually asking myself if this was a good idea or not.
So if you ask for 1 as the lower bound, potentially the first tx needs to be at least in slot 2. If we put >= :low!
here and there is no block in slot 1, it would find the last tx in slot 2, and we would end up skipping it because of the other condition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably worth adding a comment to explain this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
accepting modulo maybe adding a comment as discussed elsewhere on this issue
No description provided.